Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 97.86% 97.92% +0.06%
==========================================
Files 60 62 +2
Lines 5859 5989 +130
==========================================
+ Hits 5734 5865 +131
+ Misses 125 124 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
its-hammer-time
force-pushed
the
implement-radix-tree-for-improved-lookups
branch
from
1d245b8 to
02a6420
Compare
its-hammer-time
force-pushed
the
implement-radix-tree-for-improved-lookups
branch
4 times, most recently
from
d2bfaf7 to
f7a0b49
Compare
its-hammer-time
force-pushed
the
implement-radix-tree-for-improved-lookups
branch
from
f7a0b49 to
c4c00c4
Compare
|
I came to review this and I see missing coverage and it needs a rebase :) so I stopped. |
daveshanley
left a comment
daveshanley
left a comment
There was a problem hiding this comment.
Can we remove deprecated stuff? We need fresh, hot code.. not old cold code.
|
@its-hammer-time ^^ you coming back on this one? |
Co-authored-by: Cursor <cursoragent@cursor.com>
its-hammer-time
force-pushed
the
implement-radix-tree-for-improved-lookups
branch
from
124aee1 to
acfc668
Compare
@daveshanley sorry for the delay. Updated those years to 2026 and changed to |
Cover WithPathTree, DisablePathTree, IsPathTreeDisabled, and the FindPath radix tree fast-path branches (method match and mismatch). Made-with: Cursor
3 participants
NOTICE
This PR has a subtle breaking change when it comes to identical paths. Refer to the comment below for more info:
https://github.com/pb33f/libopenapi-validator/pull/217/changes#r2700417695
Summary
I noticed some performance issues with the current path matching approach as it recomputes regexes on every validation call. I unfortunately didn't notice the regex cache before making this change, but either way according to my benchmarks this approach is faster and more memory efficient.
The idea is to essentially use a Radix tree for matching the incoming request path against those in the OpenAPI spec to find the appropriate PathItem. The RadixTree will bring the runtime down to O(k) where k is the number of path segments; however, it does not work for the more complex paths (OData, matrix, etc.). So I've essentially made it so we try and run the RadixTree lookup first as I imagine most HTTP urls are simple paths and if that fails we then fall back to the previous approach.
Note that most of the changes in this PR are due to me updating the
FindPathfunction to take in the ValidatorOptions rather than RegexCache directly. I either had to add the Radix tree as a new arg or swap to options so I figured swapping to options would reduce changes in the future if we needed more things later.Testing
AI wrote all of the tests, but I had it leave all of the previous unit tests except for a couple of outliers:
path_parameter_test.go- The tests that were removed in this file were testing that the RegexCache was appropriately updated for the simple path test case; however, we never hit the cache anymore since the radix handles that case for us. Instead, a new test was added with a supported (simple) and unsupported (OData) path and we test that the unsupported path does have a cache hit.paths_test.go- Same story here honestly. The only thing that changes is if it was testing the regex cache then we have to move it to an unsupported path. If it's a simple path, none of the tests should have to change, it's backwards compatible. No one should really notice anything is different (except for performance 😄)